-
Notifications
You must be signed in to change notification settings - Fork 2.1k
[CHIA-3793] after the hard fork, disallow block references #20218
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
82dab9f to
e90d3c4
Compare
e90d3c4 to
f4a0b5e
Compare
|
| # 8c. The generator ref list must not point to a height >= this block's height | ||
| if block.transactions_generator_ref_list in (None, []): | ||
| if block.transactions_generator_ref_list == []: | ||
| if block.transactions_info.generator_refs_root != bytes([1] * 32): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is such a bizarre magic number. I'm very puzzled why we wouldn't just use the serialization for the empty list. But I don't suppose it much matters now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't a list, it's a bytes32. it must always be 32 bytes in the serialization. Still, It could be a named constant.
| # 8b. The generator ref list length must be less than or equal to MAX_GENERATOR_REF_LIST_SIZE entries | ||
| # 8c. The generator ref list must not point to a height >= this block's height | ||
| if block.transactions_generator_ref_list in (None, []): | ||
| if block.transactions_generator_ref_list == []: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just a tidy-up, or is it required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct, this is unrelated. I was a bit surprised mypy didn't flag this. transaction_generator_ref_list is not Optional[]. It can't be None.
|
|
||
| # With hard fork 2 we ban transactions_generator_ref_list. | ||
| if prev_tx_height >= self.constants.HARD_FORK2_HEIGHT and block.transactions_generator_ref_list != []: | ||
| return None, Err.TOO_MANY_GENERATOR_REFS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit puzzled as to why this is checked in multiple places. Maybe one is for all blocks and another is just for "transaction blocks"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We check it in BlockTools, to make sure no test is trying to create an invalid block.
We check it in block body validation, which is the main block validation function.
We also check it when validating unfinished blocks, which is a bit trickier because the height of the block isn't as well known.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a good understanding of this consensus code, but there's nothing in this PR that seems terribly wrong. I guess we just need some tests.
Purpose:
With hard fork 2, we want to disallow block references. We have not seen any benefit or use of them so far. We currently compress block generators using (internal) back references. Deduplicating structures.
Disallowing block generators has the following upsides:
There are two additional commits in this PR.